fix: Windows PATH and terminal DPI scaling improvements #1262
Conversation
|
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR adds platform-aware memory directory resolution and LadybugDB installation status tracking throughout the system. It introduces an IPC handler to dynamically retrieve system-specific memories paths, replaces hardcoded paths with dynamic values in environment and UI components, extends memory service status with LadybugDB health checks, and improves Windows terminal command escaping for PowerShell compatibility. Changes
Sequence DiagramsequenceDiagram
participant Renderer as Renderer<br/>(React Component)
participant IPC as IPC Bridge
participant Main as Main Process<br/>(Memory Service)
participant FS as File System<br/>(Python/Environment)
Renderer->>Renderer: Component mounted
Renderer->>IPC: invoke MEMORY_GET_DIR
IPC->>Main: handle MEMORY_GET_DIR
Main->>Main: getDefaultDbPath()
Main-->>IPC: return path (e.g., ~/.auto-claude/memories)
IPC-->>Renderer: { success: true, data: path }
Renderer->>Renderer: Update memoriesDir state
Renderer->>IPC: invoke infrastructure status check
IPC->>Main: handle infrastructure check
Main->>FS: checkLadybugInstalled()
Main->>FS: Execute version check command
FS-->>Main: stdout/stderr result
Main->>Main: Parse result, cache status
Main-->>IPC: { memory: { ladybugInstalled, ladybugError } }
IPC-->>Renderer: Status with LadybugDB info
Renderer->>Renderer: Update UI with status/errors
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @mateuszruszkowski, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the developer experience for Windows users by resolving two critical issues. It ensures that Windows Bash sessions have complete access to system-wide tools by correctly propagating the full system PATH, and it introduces dynamic DPI-aware font scaling for the terminal, providing a consistent and readable interface across various display configurations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
I have read the CLA Document and I hereby sign the CLA |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thanks for your first PR!
A maintainer will review it soon. Please make sure:
- Your branch is synced with
develop - CI checks pass
- You've followed our contribution guide
Welcome to the Auto Claude community!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| // Calculate optimal font size based on device pixel ratio (Windows DPI scaling) | ||
| // devicePixelRatio > 1 indicates high-DPI displays (125%, 150%, 200% scaling) | ||
| // Base size: 13px for 100% scaling (devicePixelRatio = 1) | ||
| const baseFontSize = 13; | ||
| const scaleFactor = window.devicePixelRatio || 1; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| // Handle DPI/scale changes (Windows display scaling, moving between monitors) | ||
| useEffect(() => { | ||
| const handleDPIChange = debounce(() => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces two valuable improvements for Windows users: providing the full system PATH to Bash sessions and implementing DPI-aware font scaling in the terminal. These changes will significantly enhance the developer experience.
My review focuses on refining the implementation. I've suggested reusing an existing helper function in the Python backend to simplify the PATH retrieval logic. For the frontend, I've identified a bug in the DPI change handler that prevents it from detecting subsequent DPI changes and proposed a fix. I've also pointed out some opportunities to improve code clarity and reduce duplication in the font scaling calculations. Overall, great work on tackling these platform-specific issues.
| useEffect(() => { | ||
| const handleDPIChange = debounce(() => { | ||
| if (xtermRef.current) { | ||
| const baseFontSize = 13; | ||
| const scaleFactor = window.devicePixelRatio || 1; | ||
| const adjustedFontSize = scaleFactor > 1 | ||
| ? Math.max(baseFontSize * (1 / Math.sqrt(scaleFactor)), 11) | ||
| : baseFontSize; | ||
|
|
||
| // Update font size dynamically | ||
| xtermRef.current.options.fontSize = Math.round(adjustedFontSize); | ||
|
|
||
| // Trigger a fit to recalculate terminal dimensions with new font size | ||
| if (fitAddonRef.current && terminalRef.current) { | ||
| const rect = terminalRef.current.getBoundingClientRect(); | ||
| if (rect.width > 0 && rect.height > 0) { | ||
| fitAddonRef.current.fit(); | ||
| } | ||
| } | ||
| } | ||
| }, 200); // Debounce DPI changes to avoid excessive recalculations | ||
|
|
||
| // Listen for DPI/scale changes via matchMedia | ||
| // This fires when user moves window between monitors with different DPI | ||
| // or changes Windows display scaling settings | ||
| const mediaQuery = window.matchMedia(`(resolution: ${window.devicePixelRatio}dppx)`); | ||
|
|
||
| // Modern API: addEventListener | ||
| if (mediaQuery.addEventListener) { | ||
| mediaQuery.addEventListener('change', handleDPIChange); | ||
| return () => mediaQuery.removeEventListener('change', handleDPIChange); | ||
| } | ||
|
|
||
| // Fallback for older browsers: addListener (deprecated) | ||
| if (mediaQuery.addListener) { | ||
| mediaQuery.addListener(handleDPIChange); | ||
| return () => mediaQuery.removeListener?.(handleDPIChange); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
This DPI change handler has a bug: it only detects the first DPI change. The mediaQuery is created once with the initial devicePixelRatio and is not updated after a change, so subsequent DPI changes are missed.
To fix this, the useEffect hook needs to re-run to create a new MediaQueryList with the updated devicePixelRatio. You can achieve this by introducing a state variable to track the pixel ratio.
- Add state to your component:
const [dpr, setDpr] = useState(window.devicePixelRatio || 1); - Update this
useEffectto depend ondprand update it within the handler. Using{ once: true }for the event listener simplifies cleanup and re-registration.
Additionally, the font size calculation logic is duplicated from the initialization effect. When refactoring, please extract this logic into a shared helper function to follow the DRY principle.
// Handle DPI/scale changes (Windows display scaling, moving between monitors)
useEffect(() => {
const handleDPIChange = debounce(() => {
if (xtermRef.current) {
const newScaleFactor = window.devicePixelRatio || 1;
const baseFontSize = 13;
const adjustedFontSize = newScaleFactor > 1
? Math.max(baseFontSize * (1 / Math.sqrt(newScaleFactor)), 11)
: baseFontSize;
xtermRef.current.options.fontSize = Math.round(adjustedFontSize);
if (fitAddonRef.current && terminalRef.current) {
const rect = terminalRef.current.getBoundingClientRect();
if (rect.width > 0 && rect.height > 0) {
fitAddonRef.current.fit();
}
}
setDpr(newScaleFactor);
}
}, 200);
const mediaQuery = window.matchMedia(`(resolution: ${dpr}dppx)`);
mediaQuery.addEventListener('change', handleDPIChange, { once: true });
return () => {
mediaQuery.removeEventListener('change', handleDPIChange);
};
}, [dpr]);| # Get PATH from environment - Windows is case-insensitive for env vars | ||
| path = os.environ.get("PATH") or os.environ.get("Path") or os.environ.get("path") | ||
| return path |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication, this logic for retrieving the PATH environment variable on Windows can be simplified by using the existing get_env_var helper function from this same file, which already handles case-insensitive lookups correctly.
| # Get PATH from environment - Windows is case-insensitive for env vars | |
| path = os.environ.get("PATH") or os.environ.get("Path") or os.environ.get("path") | |
| return path | |
| # Use the existing helper for case-insensitive environment variable access. | |
| return get_env_var("PATH") |
| // Adjust font size inversely to pixel ratio to maintain visual consistency | ||
| // For 150% scaling (1.5x), use 13 * (1 / 1.5) ≈ 8.67 logical pixels |
There was a problem hiding this comment.
This comment is misleading as it describes a direct inverse scaling (1 / 1.5), but the implementation on line 57 uses 1 / Math.sqrt(scaleFactor). To prevent confusion for future maintainers, please update the comment to accurately reflect the code's logic, which uses a square root to make the font size reduction less aggressive.
| // Adjust font size inversely to pixel ratio to maintain visual consistency | |
| // For 150% scaling (1.5x), use 13 * (1 / 1.5) ≈ 8.67 logical pixels | |
| // Adjust font size using the square root of the scale factor to make the | |
| // font size reduction less aggressive on high-DPI displays. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/backend/core/platform/__init__.py`:
- Around line 519-535: The get_windows_system_path function duplicates
case-insensitive env lookup; replace the manual
os.environ.get("PATH")/("Path")/("path") logic by calling the existing helper
get_env_var("PATH") so behavior is centralized; update the return to use
get_env_var("PATH") inside get_windows_system_path and keep the early
is_windows() check intact.
In `@apps/frontend/src/main/env-utils.ts`:
- Around line 111-127: COMMON_BIN_PATHS currently contains hardcoded Windows
roots; replace those literals by constructing Windows paths from environment
variables and platform helpers: when process.platform === 'win32', read
ProgramFiles, ProgramFiles(x86) (or fallback to ProgramFiles), ProgramData,
SystemRoot, LOCALAPPDATA, APPDATA and USERPROFILE, use path.join or path.resolve
to build the specific subpaths (e.g. join(ProgramFiles, 'Git', 'cmd'),
join(ProgramFiles, 'nodejs'), join(LOCALAPPDATA, 'Programs', 'nodejs'),
join(APPDATA, 'npm'), join(USERPROFILE, '.local', 'bin'), join(ProgramData,
'chocolatey', 'bin'), etc.), expand '~' against USERPROFILE for POSIX-style
entries, filter out undefined or non-existent env values, and only include these
constructed entries in COMMON_BIN_PATHS so platform-specific roots are not
hardcoded.
In `@apps/frontend/src/renderer/components/terminal/useXterm.ts`:
- Around line 333-372: Guard against window.matchMedia being undefined and add a
resize fallback, then re-register the media query whenever DPI changes: inside
the effect, check if window.matchMedia exists before using it and fall back to
listening to window.resize; modify the handleDPIChange routine (and its
debounce) to, after adjusting xtermRef.current.options.fontSize and calling
fitAddonRef.current.fit() (using terminalRef to check bounds), recreate the
mediaQuery using the latest window.devicePixelRatio and reattach its 'change'
listener so the query isn't stale; ensure you remove the correct listeners on
cleanup (use removeEventListener when added and removeListener fallback for
older APIs) and reference the existing symbols handleDPIChange, xtermRef,
fitAddonRef, terminalRef, and mediaQuery when updating the logic.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| if (mediaQuery.addListener) { | ||
| mediaQuery.addListener(handleDPIChange); | ||
| return () => mediaQuery.removeListener?.(handleDPIChange); | ||
| } |
There was a problem hiding this comment.
DPI change detection misses multi-monitor transitions
Medium Severity
The matchMedia query is created once with the initial devicePixelRatio, but the change event only fires when the match state changes. Moving between two DPI values that are both different from the initial one (e.g., 2x → 1x when started at 1.5x) won't trigger the handler because the query remains in "not matching" state throughout. The correct pattern requires recreating the media query after each DPI change using { once: true } and re-registering a new listener for the updated ratio.
apps/frontend/src/main/env-utils.ts
Outdated
| win32: [ | ||
| // Windows usually handles PATH better, but we can add common locations | ||
| // User-local binaries | ||
| '~/.local/bin', // User-local binaries (Claude CLI installed here) |
There was a problem hiding this comment.
Windows path uses inconsistent forward slash separator
Medium Severity
The newly added '~/.local/bin' path uses a forward slash, while all other Windows tilde-prefixed paths use backslashes (e.g., '~\\AppData\\Roaming\\npm'). After expansion with os.homedir() (which returns backslashes on Windows), this creates mixed separators like 'C:\\Users\\username/.local/bin'. The currentPathSet.has(p) check at line 238 uses string comparison, so this won't match an existing PATH entry 'C:\\Users\\username\\.local\\bin', causing potential duplicate PATH entries.
🤖 Auto Claude PR ReviewMerge Verdict: 🔴 BLOCKED
Blocked: 4 CI check(s) failing. Fix CI before merge. Risk Assessment
🚨 Blocking Issues (Must Fix)
Generated by Auto Claude PR Review This review identified blockers that must be resolved before merge. Generated by Auto Claude. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/frontend/src/renderer/components/terminal/useXterm.ts`:
- Around line 383-399: Guard against window.matchMedia being undefined and make
the DPI listener re-register the media query after DPI changes: in the effect
that creates mediaQuery (the variable mediaQuery) check if typeof
window.matchMedia === 'function' before using it and return early if not (or add
the provided JSDOM mock in test setup to unblock CI); modify handleDPIChange so
it removes listeners from the existing mediaQuery, recomputes a new mediaQuery
using the current window.devicePixelRatio, and attaches the appropriate listener
via addEventListener/addListener (and corresponding
removeEventListener/removeListener) so subsequent DPI changes (e.g., moving
between monitors) are detected; ensure cleanup returns a function that removes
listeners from whichever mediaQuery is currently registered.
♻️ Duplicate comments (2)
apps/frontend/src/renderer/components/terminal/useXterm.ts (2)
54-56: Comment does not match implementation.The comment describes direct inverse scaling (
1 / 1.5), but the code uses1 / Math.sqrt(scaleFactor). Update the comment to accurately reflect the square-root-based formula.
364-368: Extract duplicated font size calculation to a helper function.This calculation is duplicated from lines 52-58. Extract to a reusable helper to follow DRY and ensure consistency if the formula changes.
♻️ Suggested refactor
Add a helper function at the top of the file (after imports):
/** Calculate DPI-adjusted font size for terminal */ function getAdjustedFontSize(baseFontSize = 13, minFontSize = 11): number { const scaleFactor = window.devicePixelRatio || 1; const adjustedFontSize = scaleFactor > 1 ? Math.max(baseFontSize * (1 / Math.sqrt(scaleFactor)), minFontSize) : baseFontSize; return Math.round(adjustedFontSize); }Then use it in both locations:
- const baseFontSize = 13; - const scaleFactor = window.devicePixelRatio || 1; - const adjustedFontSize = scaleFactor > 1 - ? Math.max(baseFontSize * (1 / Math.sqrt(scaleFactor)), 11) - : baseFontSize; + const adjustedFontSize = getAdjustedFontSize();
apps/backend/core/auth.py
Outdated
| # in Bash because they're not in the limited COMMON_BIN_PATHS | ||
| if platform.system() == "Windows": | ||
| system_path = get_windows_system_path() | ||
| if system_path: | ||
| env["PATH"] = system_path |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…dows The Memory settings section was displaying Unix-style paths (~/.auto-claude/memories) instead of Windows paths (C:\Users\<user>\.auto-claude\memories). This fix: - Adds new IPC handler (MEMORY_GET_DIR) to return platform-specific memories path - Updates MemoryBackendSection, SecuritySettings, MemoriesTab to fetch and display correct path - Updates GraphitiStep (onboarding wizard) to show correct platform path - Fixes path separator in memory-service.ts error message (uses path.join instead of /) - Updates env-handlers to use dynamic path in .env template - Removes trailing slashes from path displays to avoid mixed separators Co-Authored-By: Claude Opus 4.5 <[email protected]>
… messages - Add checkLadybugInstalled() function to detect if real_ladybug Python package is installed - Show detailed error message in Setup Wizard when LadybugDB is not installed - Include link to Visual Studio Build Tools download for Windows users - Update InfrastructureStatus component with same error handling - Add ladybugInstalled and ladybugError fields to MemoryServiceStatus interface On Windows, LadybugDB requires Visual Studio Build Tools to compile. This change provides clear guidance to users when installation fails. Co-Authored-By: Claude Opus 4.5 <[email protected]>
PowerShell interprets -- flags as the pre-decrement operator, causing errors like "The '--' operator works only on variables or on properties". Changes: - Add shellType parameter to escapeShellCommand function - Use call operator (&) prefix for PowerShell commands: & "cmd.exe" - Update all 4 escapeShellCommand calls to pass terminal.shellType - Add comprehensive tests for escapeShellCommand function This fixes Claude CLI invocation when using PowerShell as the terminal. Co-Authored-By: Claude Opus 4.5 <[email protected]>
mateuszruszkowski
force-pushed
the
fix/windows-path-and-terminal-scaling
branch
from
53d3373 to
931badd
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/frontend/src/renderer/lib/mocks/infrastructure-mock.ts (1)
82-82: Hardcoded Unix path in mock data.Line 82 contains
/usr/local/bin/ollama, which is a Unix-specific path. While this is mock data and not execution logic, consider using a platform-agnostic placeholder or noting this is a Unix-only mock. As per coding guidelines, platform-specific paths should be avoided.apps/frontend/src/main/memory-service.ts (1)
144-146: Use platform abstraction instead ofprocess.platform.Per coding guidelines, avoid checking
process.platformdirectly. Import and useisWindows()from the platform module instead.♻️ Proposed fix
Add the import at the top:
+import { isWindows } from './platform';Then update the code:
- const venvPython = process.platform === 'win32' - ? path.join(backendPath, '.venv', 'Scripts', 'python.exe') - : path.join(backendPath, '.venv', 'bin', 'python'); + const venvPython = isWindows() + ? path.join(backendPath, '.venv', 'Scripts', 'python.exe') + : path.join(backendPath, '.venv', 'bin', 'python');apps/frontend/src/renderer/components/onboarding/GraphitiStep.tsx (1)
828-898: Replace newly added user-facing strings with i18n keys.
These newly introduced UI strings should usereact-i18nextkeys rather than hardcoded text.As per coding guidelines, and based on learnings, this only flags the newly introduced strings.✅ Proposed fix (apply to new strings)
-import { useState, useEffect } from 'react'; +import { useState, useEffect } from 'react'; +import { useTranslation } from 'react-i18next'; export function GraphitiStep({ onNext, onBack, onSkip }: GraphitiStepProps) { + const { t } = useTranslation(); const { settings, updateSettings } = useSettingsStore(); ... - LadybugDB Not Installed + {t('onboarding:graphiti.ladybugNotInstalled.title')} ... - Download Visual Studio Build Tools + {t('onboarding:graphiti.ladybugNotInstalled.downloadBuildTools')} ... - After installing build tools, restart the application to retry. + {t('onboarding:graphiti.ladybugNotInstalled.restartNotice')} ... - Database will be created automatically + {t('onboarding:graphiti.databaseWillBeCreated.title')} ... - LadybugDB uses an embedded database - no Docker required. - The database will be created when you first use memory features. + {t('onboarding:graphiti.databaseWillBeCreated.body')} ... - LadybugDB Ready + {t('onboarding:graphiti.ladybugReady.title')} ... - Memory database is installed and available. + {t('onboarding:graphiti.ladybugReady.body')} ... - Stored in {memoriesDir || 'memories directory'} + {t('onboarding:graphiti.storedIn', { + path: memoriesDir || t('onboarding:graphiti.memoriesDirFallback') + })}Also applies to: 994-995
🤖 Fix all issues with AI agents
In `@apps/frontend/src/main/memory-service.ts`:
- Around line 779-784: The code uses a dynamic require to get spawnSync; replace
that with a static import at the top of the module alongside spawn from
'child_process' and remove the require call where spawnSync is used (the block
that currently declares const { spawnSync } = require('child_process'); and then
calls spawnSync). Update imports to statically import { spawn, spawnSync } from
'child_process' so both symbols are available for the code that calls
spawnSync(cmd, checkArgs, ...).
In
`@apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts`:
- Around line 646-691: Add a Linux-specific unit test to the escapeShellCommand
suite: inside the existing describe('escapeShellCommand') block add a test that
calls mockPlatform('linux'), imports escapeShellCommand from
../claude-integration-handler, calls escapeShellCommand('/usr/local/bin/claude')
and asserts the result equals "'/usr/local/bin/claude'"; this mirrors the macOS
(darwin) case and ensures Linux coverage alongside Windows and macOS.
In `@apps/frontend/src/main/terminal/claude-integration-handler.ts`:
- Around line 97-120: The PowerShell branch in escapeShellCommand should use
PowerShell-safe single-quote quoting instead of escapeForWindowsDoubleQuote:
when isWindows() and shellType === 'powershell', produce a single-quoted string
(handle embedded single quotes by doubling them) and prefix with the call
operator (&) so the result is & '...'; do not apply cmd.exe-specific percent (%)
escaping or double-quote escaping (escapeForWindowsDoubleQuote) in this
branch—leave escapeForWindowsDoubleQuote for the cmd.exe branch only.
In `@apps/frontend/src/renderer/components/project-settings/SecuritySettings.tsx`:
- Around line 57-76: Extract the duplicated memoriesDir state and fetch logic
into a reusable hook (e.g., useMemoriesDir) and replace the local
useState/useEffect in SecuritySettings, MemoryBackendSection, and MemoriesTab
with that hook; specifically, move the window.electronAPI.getMemoriesDir() call
and the setMemoriesDir updater into the new useMemoriesDir hook and have it
return the memoriesDir string, then in the components remove the local
memoriesDir/setMemoriesDir and import/use useMemoriesDir() where the current
memoriesDir variable is referenced.
♻️ Duplicate comments (2)
apps/frontend/src/renderer/components/project-settings/MemoryBackendSection.tsx (1)
51-52: Duplicate memoriesDir fetch logic.This is the same pattern as in
SecuritySettings.tsx. As noted in the previous file review, consider extracting this to a shareduseMemoriesDir()hook.Also applies to: 96-107
apps/frontend/src/renderer/components/context/MemoriesTab.tsx (1)
82-95: Duplicate memoriesDir fetch logic.Same pattern as in
SecuritySettings.tsxandMemoryBackendSection.tsx. A shareduseMemoriesDir()hook would eliminate this duplication.
| const { spawnSync } = require('child_process'); | ||
| const result = spawnSync(cmd, checkArgs, { | ||
| encoding: 'utf-8', | ||
| timeout: 10000, | ||
| windowsHide: true, | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use static import instead of dynamic require.
spawnSync should be imported at the top of the file alongside spawn from child_process rather than using a dynamic require call. This is cleaner and more consistent with the existing imports.
♻️ Proposed fix
At the top of the file (line 10), update the import:
-import { spawn } from 'child_process';
+import { spawn, spawnSync } from 'child_process';Then at line 779:
- const { spawnSync } = require('child_process');
- const result = spawnSync(cmd, checkArgs, {
+ const result = spawnSync(cmd, checkArgs, {🤖 Prompt for AI Agents
In `@apps/frontend/src/main/memory-service.ts` around lines 779 - 784, The code
uses a dynamic require to get spawnSync; replace that with a static import at
the top of the module alongside spawn from 'child_process' and remove the
require call where spawnSync is used (the block that currently declares const {
spawnSync } = require('child_process'); and then calls spawnSync). Update
imports to statically import { spawn, spawnSync } from 'child_process' so both
symbols are available for the code that calls spawnSync(cmd, checkArgs, ...).
| describe('escapeShellCommand', () => { | ||
| it('should add & call operator for PowerShell on Windows', async () => { | ||
| mockPlatform('win32'); | ||
| const { escapeShellCommand } = await import('../claude-integration-handler'); | ||
|
|
||
| // PowerShell needs & to execute commands with -- flags | ||
| // Without &, PowerShell interprets -- as the decrement operator | ||
| const result = escapeShellCommand('C:\\Users\\test\\claude.exe', 'powershell'); | ||
| expect(result).toBe('& "C:\\Users\\test\\claude.exe"'); | ||
| }); | ||
|
|
||
| it('should NOT add & call operator for cmd.exe on Windows', async () => { | ||
| mockPlatform('win32'); | ||
| const { escapeShellCommand } = await import('../claude-integration-handler'); | ||
|
|
||
| const result = escapeShellCommand('C:\\Users\\test\\claude.exe', 'cmd'); | ||
| expect(result).toBe('"C:\\Users\\test\\claude.exe"'); | ||
| expect(result).not.toContain('&'); | ||
| }); | ||
|
|
||
| it('should default to cmd.exe style when shellType is undefined on Windows', async () => { | ||
| mockPlatform('win32'); | ||
| const { escapeShellCommand } = await import('../claude-integration-handler'); | ||
|
|
||
| const result = escapeShellCommand('C:\\Users\\test\\claude.exe'); | ||
| expect(result).toBe('"C:\\Users\\test\\claude.exe"'); | ||
| expect(result).not.toContain('&'); | ||
| }); | ||
|
|
||
| it('should use single quotes on Unix', async () => { | ||
| mockPlatform('darwin'); | ||
| const { escapeShellCommand } = await import('../claude-integration-handler'); | ||
|
|
||
| const result = escapeShellCommand('/usr/local/bin/claude'); | ||
| expect(result).toBe("'/usr/local/bin/claude'"); | ||
| }); | ||
|
|
||
| it('should escape special characters in PowerShell path', async () => { | ||
| mockPlatform('win32'); | ||
| const { escapeShellCommand } = await import('../claude-integration-handler'); | ||
|
|
||
| // Paths with special chars like % should be escaped | ||
| const result = escapeShellCommand('C:\\Users\\test%user\\claude.exe', 'powershell'); | ||
| expect(result).toBe('& "C:\\Users\\test%%user\\claude.exe"'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add Linux coverage for escapeShellCommand tests.
These new tests exercise Windows and macOS, but not Linux. Please add a Linux case to satisfy the “all three platforms” test rule.
✅ Suggested test addition
it('should use single quotes on Unix', async () => {
mockPlatform('darwin');
const { escapeShellCommand } = await import('../claude-integration-handler');
const result = escapeShellCommand('/usr/local/bin/claude');
expect(result).toBe("'/usr/local/bin/claude'");
});
+
+ it('should use single quotes on Linux', async () => {
+ mockPlatform('linux');
+ const { escapeShellCommand } = await import('../claude-integration-handler');
+
+ const result = escapeShellCommand('/usr/local/bin/claude');
+ expect(result).toBe("'/usr/local/bin/claude'");
+ });As per coding guidelines, please ensure platform-specific tests cover Windows, macOS, and Linux.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe('escapeShellCommand', () => { | |
| it('should add & call operator for PowerShell on Windows', async () => { | |
| mockPlatform('win32'); | |
| const { escapeShellCommand } = await import('../claude-integration-handler'); | |
| // PowerShell needs & to execute commands with -- flags | |
| // Without &, PowerShell interprets -- as the decrement operator | |
| const result = escapeShellCommand('C:\\Users\\test\\claude.exe', 'powershell'); | |
| expect(result).toBe('& "C:\\Users\\test\\claude.exe"'); | |
| }); | |
| it('should NOT add & call operator for cmd.exe on Windows', async () => { | |
| mockPlatform('win32'); | |
| const { escapeShellCommand } = await import('../claude-integration-handler'); | |
| const result = escapeShellCommand('C:\\Users\\test\\claude.exe', 'cmd'); | |
| expect(result).toBe('"C:\\Users\\test\\claude.exe"'); | |
| expect(result).not.toContain('&'); | |
| }); | |
| it('should default to cmd.exe style when shellType is undefined on Windows', async () => { | |
| mockPlatform('win32'); | |
| const { escapeShellCommand } = await import('../claude-integration-handler'); | |
| const result = escapeShellCommand('C:\\Users\\test\\claude.exe'); | |
| expect(result).toBe('"C:\\Users\\test\\claude.exe"'); | |
| expect(result).not.toContain('&'); | |
| }); | |
| it('should use single quotes on Unix', async () => { | |
| mockPlatform('darwin'); | |
| const { escapeShellCommand } = await import('../claude-integration-handler'); | |
| const result = escapeShellCommand('/usr/local/bin/claude'); | |
| expect(result).toBe("'/usr/local/bin/claude'"); | |
| }); | |
| it('should escape special characters in PowerShell path', async () => { | |
| mockPlatform('win32'); | |
| const { escapeShellCommand } = await import('../claude-integration-handler'); | |
| // Paths with special chars like % should be escaped | |
| const result = escapeShellCommand('C:\\Users\\test%user\\claude.exe', 'powershell'); | |
| expect(result).toBe('& "C:\\Users\\test%%user\\claude.exe"'); | |
| }); | |
| }); | |
| describe('escapeShellCommand', () => { | |
| it('should add & call operator for PowerShell on Windows', async () => { | |
| mockPlatform('win32'); | |
| const { escapeShellCommand } = await import('../claude-integration-handler'); | |
| // PowerShell needs & to execute commands with -- flags | |
| // Without &, PowerShell interprets -- as the decrement operator | |
| const result = escapeShellCommand('C:\\Users\\test\\claude.exe', 'powershell'); | |
| expect(result).toBe('& "C:\\Users\\test\\claude.exe"'); | |
| }); | |
| it('should NOT add & call operator for cmd.exe on Windows', async () => { | |
| mockPlatform('win32'); | |
| const { escapeShellCommand } = await import('../claude-integration-handler'); | |
| const result = escapeShellCommand('C:\\Users\\test\\claude.exe', 'cmd'); | |
| expect(result).toBe('"C:\\Users\\test\\claude.exe"'); | |
| expect(result).not.toContain('&'); | |
| }); | |
| it('should default to cmd.exe style when shellType is undefined on Windows', async () => { | |
| mockPlatform('win32'); | |
| const { escapeShellCommand } = await import('../claude-integration-handler'); | |
| const result = escapeShellCommand('C:\\Users\\test\\claude.exe'); | |
| expect(result).toBe('"C:\\Users\\test\\claude.exe"'); | |
| expect(result).not.toContain('&'); | |
| }); | |
| it('should use single quotes on Unix', async () => { | |
| mockPlatform('darwin'); | |
| const { escapeShellCommand } = await import('../claude-integration-handler'); | |
| const result = escapeShellCommand('/usr/local/bin/claude'); | |
| expect(result).toBe("'/usr/local/bin/claude'"); | |
| }); | |
| it('should use single quotes on Linux', async () => { | |
| mockPlatform('linux'); | |
| const { escapeShellCommand } = await import('../claude-integration-handler'); | |
| const result = escapeShellCommand('/usr/local/bin/claude'); | |
| expect(result).toBe("'/usr/local/bin/claude'"); | |
| }); | |
| it('should escape special characters in PowerShell path', async () => { | |
| mockPlatform('win32'); | |
| const { escapeShellCommand } = await import('../claude-integration-handler'); | |
| // Paths with special chars like % should be escaped | |
| const result = escapeShellCommand('C:\\Users\\test%user\\claude.exe', 'powershell'); | |
| expect(result).toBe('& "C:\\Users\\test%%user\\claude.exe"'); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In `@apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts`
around lines 646 - 691, Add a Linux-specific unit test to the escapeShellCommand
suite: inside the existing describe('escapeShellCommand') block add a test that
calls mockPlatform('linux'), imports escapeShellCommand from
../claude-integration-handler, calls escapeShellCommand('/usr/local/bin/claude')
and asserts the result equals "'/usr/local/bin/claude'"; this mirrors the macOS
(darwin) case and ensures Linux coverage alongside Windows and macOS.
| * | ||
| * For PowerShell, adds the call operator (&) before the command to prevent | ||
| * PowerShell from interpreting -- flags as the pre-decrement operator. | ||
| * | ||
| * On Unix/macOS, wraps in single quotes for bash. | ||
| * | ||
| * @param cmd - The command to escape | ||
| * @param shellType - On Windows, specify 'powershell' or 'cmd' for correct syntax. | ||
| * @returns The escaped command safe for use in shell commands | ||
| */ | ||
| function escapeShellCommand(cmd: string): string { | ||
| export function escapeShellCommand(cmd: string, shellType?: WindowsShellType): string { | ||
| if (isWindows()) { | ||
| // Windows: Wrap in double quotes and escape only embedded double quotes | ||
| // Inside double quotes, caret is literal, so use escapeForWindowsDoubleQuote() | ||
| const escapedCmd = escapeForWindowsDoubleQuote(cmd); | ||
|
|
||
| if (shellType === 'powershell') { | ||
| // PowerShell: Use call operator (&) to execute the command | ||
| // Without &, PowerShell interprets "--flag" as pre-decrement operator | ||
| return `& "${escapedCmd}"`; | ||
| } | ||
|
|
||
| // cmd.exe: Just wrap in double quotes | ||
| return `"${escapedCmd}"`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*integration-handler*" | head -20Repository: AndyMik90/Auto-Claude
Length of output: 207
🏁 Script executed:
fd "escapeForWindowsDoubleQuote" --type fRepository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
fd "claude-integration-handler" --type fRepository: AndyMik90/Auto-Claude
Length of output: 203
🏁 Script executed:
cat -n apps/frontend/src/main/terminal/claude-integration-handler.ts | sed -n '90,125p'Repository: AndyMik90/Auto-Claude
Length of output: 1710
🏁 Script executed:
rg "escapeForWindowsDoubleQuote" -A 10 -B 2Repository: AndyMik90/Auto-Claude
Length of output: 16341
🏁 Script executed:
cat -n apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts | grep -A 50 "escapeShellCommand"Repository: AndyMik90/Auto-Claude
Length of output: 4461
🏁 Script executed:
cat -n apps/frontend/src/shared/utils/shell-escape.ts | head -80Repository: AndyMik90/Auto-Claude
Length of output: 3514
🏁 Script executed:
cat -n apps/frontend/src/shared/utils/shell-escape.ts | sed -n '79,130p'Repository: AndyMik90/Auto-Claude
Length of output: 2593
🏁 Script executed:
# Check if there are any PowerShell-specific escaping functions in the codebase
rg "PowerShell|powershell" -A 3 -B 3 apps/frontend/src/shared/utils/shell-escape.tsRepository: AndyMik90/Auto-Claude
Length of output: 1024
🏁 Script executed:
cat -n apps/frontend/src/shared/utils/shell-escape.ts | sed -n '130,145p'Repository: AndyMik90/Auto-Claude
Length of output: 948
🏁 Script executed:
# Let me check the complete escapeForWindowsDoubleQuote function
cat -n apps/frontend/src/shared/utils/shell-escape.ts | sed -n '130,150p'Repository: AndyMik90/Auto-Claude
Length of output: 1165
🏁 Script executed:
# Check how escapeShellCommand is used in the codebase
rg "escapeShellCommand" -B 2 -A 2 --type ts --type tsxRepository: AndyMik90/Auto-Claude
Length of output: 92
🏁 Script executed:
# Let me verify the PowerShell-specific test more thoroughly
cat -n apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts | sed -n '683,691p'Repository: AndyMik90/Auto-Claude
Length of output: 549
🏁 Script executed:
# Let me search for more context about how PowerShell handles special characters
rg "PowerShell" -A 5 -B 5 apps/frontend/srcRepository: AndyMik90/Auto-Claude
Length of output: 37372
🏁 Script executed:
# Check if there are any other tests that validate PowerShell escaping with special characters
rg "powershell" apps/frontend/src/main/terminal/__tests__/claude-integration-handler.test.ts -A 3 -B 3Repository: AndyMik90/Auto-Claude
Length of output: 675
Use PowerShell-safe escaping when shellType === 'powershell'.
In PowerShell, double-quoted strings trigger variable expansion ($var), command substitution, and backtick escaping. Using escapeForWindowsDoubleQuote() for PowerShell leaves $ and backticks unescaped, allowing unintended interpolation. Paths with % are also unnecessarily escaped as %% (valid only in cmd.exe).
Switch to single-quote escaping for PowerShell, which prevents all expansion. Handle only embedded single quotes by doubling them:
🔧 Suggested fix
export function escapeShellCommand(cmd: string, shellType?: WindowsShellType): string {
if (isWindows()) {
if (shellType === 'powershell') {
// PowerShell: Use single quotes to prevent variable expansion, backtick interpretation
const escapedCmd = cmd
.replace(/\r/g, '')
.replace(/\n/g, '')
.replace(/'/g, "''");
return `& '${escapedCmd}'`;
}
// cmd.exe: Wrap in double quotes and escape embedded double quotes/percents
const escapedCmd = escapeForWindowsDoubleQuote(cmd);
return `"${escapedCmd}"`;
}
// Unix/macOS: Wrap in single quotes for bash
return escapeShellArg(cmd);
}🤖 Prompt for AI Agents
In `@apps/frontend/src/main/terminal/claude-integration-handler.ts` around lines
97 - 120, The PowerShell branch in escapeShellCommand should use PowerShell-safe
single-quote quoting instead of escapeForWindowsDoubleQuote: when isWindows()
and shellType === 'powershell', produce a single-quoted string (handle embedded
single quotes by doubling them) and prefix with the call operator (&) so the
result is & '...'; do not apply cmd.exe-specific percent (%) escaping or
double-quote escaping (escapeForWindowsDoubleQuote) in this branch—leave
escapeForWindowsDoubleQuote for the cmd.exe branch only.
| // Platform-specific memories directory path | ||
| const [memoriesDir, setMemoriesDir] = useState<string>(''); | ||
|
|
||
| // Sync parent's showOpenAIKey prop to local state | ||
| useEffect(() => { | ||
| setShowApiKey(prev => ({ ...prev, openai: showOpenAIKey })); | ||
| }, [showOpenAIKey]); | ||
|
|
||
| // Fetch platform-specific memories directory path | ||
| useEffect(() => { | ||
| window.electronAPI.getMemoriesDir() | ||
| .then((result) => { | ||
| if (result.success && result.data) { | ||
| setMemoriesDir(result.data); | ||
| } | ||
| }) | ||
| .catch((err) => { | ||
| console.error('Failed to get memories directory:', err); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting the memoriesDir fetch logic to a custom hook.
This same pattern (state + useEffect to fetch memoriesDir) is duplicated in SecuritySettings, MemoryBackendSection, and MemoriesTab. Extract to a reusable hook like useMemoriesDir() to reduce duplication and ensure consistency.
♻️ Example custom hook
// hooks/useMemoriesDir.ts
import { useState, useEffect } from 'react';
export function useMemoriesDir(): string {
const [memoriesDir, setMemoriesDir] = useState<string>('');
useEffect(() => {
window.electronAPI.getMemoriesDir()
.then((result) => {
if (result.success && result.data) {
setMemoriesDir(result.data);
}
})
.catch((err) => {
console.error('Failed to get memories directory:', err);
});
}, []);
return memoriesDir;
}Then in components:
-const [memoriesDir, setMemoriesDir] = useState<string>('');
-
-useEffect(() => {
- window.electronAPI.getMemoriesDir()
- .then((result) => {
- if (result.success && result.data) {
- setMemoriesDir(result.data);
- }
- })
- .catch((err) => {
- console.error('Failed to get memories directory:', err);
- });
-}, []);
+const memoriesDir = useMemoriesDir();🤖 Prompt for AI Agents
In `@apps/frontend/src/renderer/components/project-settings/SecuritySettings.tsx`
around lines 57 - 76, Extract the duplicated memoriesDir state and fetch logic
into a reusable hook (e.g., useMemoriesDir) and replace the local
useState/useEffect in SecuritySettings, MemoryBackendSection, and MemoriesTab
with that hook; specifically, move the window.electronAPI.getMemoriesDir() call
and the setMemoriesDir updater into the new useMemoriesDir hook and have it
return the memoriesDir string, then in the components remove the local
memoriesDir/setMemoriesDir and import/use useMemoriesDir() where the current
memoriesDir variable is referenced.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
🔴 Follow-up Review: Blocked
🔴 Blocked - 4 CI check(s) failing. Fix CI before merge.
Resolution Status
- ✅ Resolved: 0 previous findings addressed
- ❌ Unresolved: 0 previous findings remain
- 🆕 New Issues: 5 new findings in recent changes
🚨 Blocking Issues
- quality: CI BLOCKER: Missing null check for window.matchMedia causes test failures in JSDOM
- quality: DPI change listener will not fire on subsequent DPI changes after the first
- quality: [FROM COMMENTS] CI Blocker: window.matchMedia is undefined in JSDOM test environment
Verdict
BLOCKED: 4 CI checks failing (test-frontend on all platforms + CI Complete).
The root cause is identified: window.matchMedia is called without a guard at useXterm.ts:386, which throws TypeError: window.matchMedia is not a function in JSDOM test environment.
Issues requiring resolution:
- CRITICAL (CI Blocker): Guard
window.matchMediacall with existence check (NEW-001/CMT-001) - HIGH: Fix stale media query - must re-register listener after DPI changes (NEW-002)
- LOW: Consider adjusting minimum font size for high-DPI displays (NEW-003)
Cross-agent validation: Both new-code-reviewer and comment-analyzer independently identified the same blocking issues, providing high confidence these are genuine bugs.
To unblock: Add guard before matchMedia call, e.g.: if (typeof window.matchMedia !== 'function') return;
Review Process
Agents invoked: new-code-reviewer, comment-analyzer
This is an AI-generated follow-up review using parallel specialist analysis with finding validation.
Findings (5 selected of 5 total)
🔴 [NEW-001] [CRITICAL] CI BLOCKER: Missing null check for window.matchMedia causes test failures in JSDOM
📁 apps/frontend/src/renderer/components/terminal/useXterm.ts:386
The code calls window.matchMedia() without first checking if it exists. JSDOM (used in tests) does not implement matchMedia, causing TypeError: window.matchMedia is not a function. This is the confirmed cause of the CI failure across all test platforms.
Suggested fix:
Add a guard check before calling matchMedia:
```typescript
if (typeof window === 'undefined' || typeof window.matchMedia !== 'function') {
return;
}
const mediaQuery = window.matchMedia(...);
#### 🟠 [NEW-002] [HIGH] DPI change listener will not fire on subsequent DPI changes after the first
📁 `apps/frontend/src/renderer/components/terminal/useXterm.ts:386`
The media query is created once with the current devicePixelRatio value. When the DPI changes and the handler fires, the old media query no longer matches the new resolution, so it will only fire once. Subsequent DPI changes (e.g., moving between monitors) won't trigger the handler.
**Suggested fix:**
After handling a DPI change, create a new media query with the updated devicePixelRatio using a recursive pattern that re-registers with the new value.
#### 🔵 [NEW-003] [LOW] DPI scaling formula may produce uncomfortably small fonts on high-DPI displays
📁 `apps/frontend/src/renderer/components/terminal/useXterm.ts:57`
The formula baseFontSize * (1 / Math.sqrt(scaleFactor)) with a minimum of 11px may produce small fonts on high-DPI displays. For 200% scaling (devicePixelRatio = 2), calculation yields ~9.19px clamped to 11px, which may be too small for comfortable terminal use.
**Suggested fix:**
Consider using a minimum font size of 12px instead of 11px, or a less aggressive scaling formula.
#### 🔵 [NEW-004] [LOW] Missing cleanup return in fallback path when event listener APIs are unavailable
📁 `apps/frontend/src/renderer/components/terminal/useXterm.ts:360`
The useEffect for DPI handling has three code paths but the implicit fallback path (when neither addEventListener nor addListener exists) does not return a cleanup function, leading to an inconsistent return type.
**Suggested fix:**
Add an explicit return statement: return () => {}; at the end of the useEffect.
#### 🔴 [CMT-001] [CRITICAL] [FROM COMMENTS] CI Blocker: window.matchMedia is undefined in JSDOM test environment
📁 `apps/frontend/src/renderer/components/terminal/useXterm.ts:386`
Both CodeRabbit and Sentry identified that the matchMedia call at line 386 is not guarded. In JSDOM test environments, window.matchMedia is undefined, causing TypeError. This breaks ALL frontend tests on macOS, Windows, and Ubuntu CI runners.
**Suggested fix:**
Guard the matchMedia call: if (typeof window !== 'undefined' && window.matchMedia) { ... } or add matchMedia mock to test setup
---
*This review was generated by Auto Claude.*
3 participants
Base Branch
developbranch (required for all feature/fix PRs)main(hotfix only - maintainers)Description
This PR fixes two Windows-specific issues that significantly improve the developer experience:
Full system PATH for Bash sessions: Windows Bash sessions now have access to all installed system tools (ddev, mkcert, chocolatey, etc.) by passing the complete Windows PATH to the SDK.
DPI-aware terminal font scaling: Terminal text now scales correctly on high-DPI displays (125%, 150%, 200% scaling) and dynamically adjusts when moving windows between monitors with different DPI settings.
Related Issue
Fixes Windows users not being able to run installed system tools from Bash sessions and terminal text rendering issues on high-DPI displays.
Type of Change
Area
Commit Message Format
Follow conventional commits:
<type>: <subject>Types: feat, fix, docs, style, refactor, test, chore
Example:
feat: add user authentication systemChecklist
developbranchPlatform Testing Checklist
CRITICAL: This project supports Windows, macOS, and Linux. Platform-specific bugs are a common source of breakage.
platform/module instead of directprocess.platformchecksNote: Changes are Windows-specific and use the centralized platform module. CI will verify no regressions on macOS/Linux.
CI/Testing Requirements
Screenshots
N/A - No UI changes, backend/terminal rendering improvements only.
Feature Toggle
use_feature_nameBreaking Changes
Breaking: No
Details: Backward compatible improvements. No API changes.
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.